chore: external merge request from Contributor#36925
chore: external merge request from Contributor#36925rahulbarwal wants to merge 20 commits intoreleasefrom
Conversation
…table_widgetV2' into external-contri/fix/button-tooltip-in-table_widgetV2
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11379477080. |
|
Deploy-Preview-URL: https://ce-36925.dp.appsmith.com |
….com/skjameela/appsmith into external-contri/fix/button-tooltip-in-table_widgetV2
…o external-contri/fix/button-tooltip-in-table_widgetV2
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request primarily involve enhancements to the styling and functionality of table widget components. Modifications include new properties and styled components in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11456391351. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx (1)
Line range hint
46-72: LGTM: Button component enhanced with tooltip.The Button component now includes an AutoToolTipComponent wrapper, improving user experience. The conditional rendering logic is updated correctly.
Consider extracting the condition
props.isCellVisible && props.action.isVisible && props.action.labelinto a separate variable for better readability:const shouldRenderButton = props.isCellVisible && props.action.isVisible && props.action.label; return ( <ActionWrapper disabled={!!props.isDisabled} onClick={(e) => { e.stopPropagation(); }} > {shouldRenderButton ? ( <AutoToolTipComponent columnType={ColumnTypes.BUTTON} title={props.action.label} > {/* ... */} </AutoToolTipComponent> ) : null} </ActionWrapper> );app/client/src/widgets/TableWidgetV2/component/TableStyledWrappers.tsx (3)
Line range hint
516-524: LGTM. Add test for disabled cell state.The addition of
isCellDisabledproperty with conditional styling enhances the visual indication of disabled cells. Consider adding a test case to verify the correct rendering of disabled cells.Would you like me to generate a test case for the disabled cell state?
Line range hint
844-860: LGTM. Consider responsive design.The new RenderOptionWrapper component improves option rendering with consistent layout. Consider implementing responsive design for smaller screen sizes to ensure usability across devices.
Consider adding media queries for smaller screens:
export const RenderOptionWrapper = styled.div<{ selected: boolean }>` // ... existing styles ... + @media (max-width: 768px) { + width: 100%; + .title { + width: calc(100% - 30px); + } + .type { + left: auto; + right: 10px; + } + } `;
Line range hint
872-875: LGTM. Enhance accessibility.The new TooltipContentWrapper component effectively manages tooltip content display. Consider adding ARIA attributes to improve accessibility for screen readers.
Enhance accessibility by adding ARIA attributes:
export const TooltipContentWrapper = styled.div<{ width?: number }>` word-break: break-all; max-width: ${(props) => props.width}px; + role: tooltip; + aria-live: polite; `;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/widgets/TableWidgetV2/component/TableStyledWrappers.tsx (1 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (2 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (6)
app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx (1)
5-10: LGTM: New imports added.The new imports for ColumnTypes and AutoToolTipComponent are correctly added and align with the changes in the Button component.
app/client/src/widgets/TableWidgetV2/component/TableStyledWrappers.tsx (2)
Line range hint
604-624: LGTM. Ensure consistent borderRadius usage.The addition of
borderRadiusproperty and refined hover states enhance the checkbox's visual feedback and customization. Verify that theborderRadiusprop is consistently applied across the application for uniform styling.#!/bin/bash # Description: Check for consistent borderRadius usage # Test: Search for borderRadius prop in styled components rg -p 'borderRadius.*\${.*}' app/client/src/widgets
476-476: LGTM. Verify layout impact.The addition of
max-width: 100%;to ActionWrapper is appropriate for full-width expansion. Ensure this doesn't adversely affect the layout of action buttons in various table configurations.✅ Verification successful
Layout impact verified. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential layout issues caused by the max-width change # Test: Search for ActionWrapper usage in table-related components rg -p 'ActionWrapper' app/client/src/widgets/TableWidgetV2Length of output: 267
Script:
#!/bin/bash # Description: Re-attempt to check for ActionWrapper usage in table-related components # Test: Search for ActionWrapper usage rg 'ActionWrapper' app/client/src/widgets/TableWidgetV2Length of output: 503
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (3)
47-51: Addition ofisButtonparameter inuseToolTipfunctionThe updated function signature accommodates button-specific tooltip behavior.
95-95: UpdateuseEffectdependency arrayIncluding
isButtonensures the effect runs whenisButtonchanges.
186-190: Handle button columns inAutoToolTipComponentThe additional condition correctly handles tooltips for button columns.
| function isButtonTextTruncated(element: HTMLElement) { | ||
| const spanElement = element.querySelector("span"); | ||
| const offsetWidth = spanElement?.offsetWidth ?? 0; | ||
| const scrollWidth = spanElement?.scrollWidth ?? 0; | ||
|
|
||
| return scrollWidth > offsetWidth; | ||
| } |
There was a problem hiding this comment.
Check for null spanElement before accessing properties
If spanElement is null, accessing its properties will cause an error.
Apply this diff to handle the null case:
function isButtonTextTruncated(element: HTMLElement) {
const spanElement = element.querySelector("span");
+ if (!spanElement) return false;
const offsetWidth = spanElement.offsetWidth;
const scrollWidth = spanElement.scrollWidth;
return scrollWidth > offsetWidth;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isButtonTextTruncated(element: HTMLElement) { | |
| const spanElement = element.querySelector("span"); | |
| const offsetWidth = spanElement?.offsetWidth ?? 0; | |
| const scrollWidth = spanElement?.scrollWidth ?? 0; | |
| return scrollWidth > offsetWidth; | |
| } | |
| function isButtonTextTruncated(element: HTMLElement) { | |
| const spanElement = element.querySelector("span"); | |
| if (!spanElement) return false; | |
| const offsetWidth = spanElement.offsetWidth; | |
| const scrollWidth = spanElement.scrollWidth; | |
| return scrollWidth > offsetWidth; | |
| } |
|
Deploy-Preview-URL: https://ce-36925.dp.appsmith.com |
….com/skjameela/appsmith into external-contri/fix/button-tooltip-in-table_widgetV2
…o external-contri/fix/button-tooltip-in-table_widgetV2
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11476436884. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx (1)
44-46: Consider using a more specific event type.The event handler could benefit from explicit typing.
- const stopPropagation = (e: React.MouseEvent<HTMLDivElement>) => { + const stopPropagation = (e: MouseEvent<HTMLDivElement, MouseEvent>) => {app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (1)
55-98: Consider reviewing theuseEffectdependency array.Including
refin the dependency array may not be necessary since refs typically don't change between renders. Also, ensure that includingchildrenandisButtonis required to avoid unnecessary re-renders.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (2 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (6)
app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx (3)
5-8: LGTM! Clean import organization.
10-10: LGTM! Appropriate tooltip component import.
49-68: 🛠️ Refactor suggestionConsider optimizing the click handler with useCallback.
The inline onClick handler in ActionWrapper could cause unnecessary re-renders.
+ const handleStopPropagation = useCallback((e: MouseEvent<HTMLDivElement, MouseEvent>) => { + e.stopPropagation(); + }, []); return ( - <ActionWrapper disabled={!!props.isDisabled} onClick={stopPropagation}> + <ActionWrapper disabled={!!props.isDisabled} onClick={handleStopPropagation}>Additionally, let's verify the tooltip implementation consistency.
✅ Verification successful
The previous pattern might have failed due to syntax. Let's try these alternative approaches to gather the necessary information about the tooltip implementation and event handling patterns.
I found the AutoToolTipComponent file. Let's examine its implementation and check for similar event handler patterns in related components to make a more informed decision about the optimization suggestion.
The useCallback optimization suggestion is valid and aligns with existing patterns
The codebase shows consistent use of useCallback for stopPropagation handlers, as seen in SelectCell.tsx. The optimization would align with the existing patterns and prevent unnecessary re-renders.
- Found similar optimized implementation in
SelectCell.tsx:const onClick = useCallback((e) => e.stopPropagation(), [])- AutoToolTipComponent implementation confirms it's a complex component with state management, making render optimization important
The original suggestion to optimize the click handler remains valid:
+ const handleStopPropagation = useCallback((e: MouseEvent<HTMLDivElement, MouseEvent>) => { + e.stopPropagation(); + }, []); return ( - <ActionWrapper disabled={!!props.isDisabled} onClick={stopPropagation}> + <ActionWrapper disabled={!!props.isDisabled} onClick={handleStopPropagation}>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent AutoToolTipComponent usage in table components ast-grep --pattern 'AutoToolTipComponent columnType={$_} title={$_}'Length of output: 70
Script:
#!/bin/bash # Let's try a different approach to find AutoToolTipComponent usage rg -U "AutoToolTipComponent.*columnType.*title" -A 2 -B 2 # Also check for the component definition rg "export.*AutoToolTipComponent" # Let's also verify if stopPropagation is used consistently rg "stopPropagation.*onClick" -A 2 -B 2Length of output: 261
Script:
#!/bin/bash # Let's check the implementation of AutoToolTipComponent cat app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx # Let's also check for similar event handler patterns in the same directory rg "const.*useCallback.*stopPropagation" app/client/src/widgets/TableWidgetV2/component/cellComponents/Length of output: 6629
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (3)
39-45: Null safety handled correctly inisButtonTextTruncated.The use of optional chaining and default values ensures that accessing properties of
spanElementwill not cause errors if it's null. Good job addressing the previous concern.
47-51:useToolTipfunction updated appropriately withisButtonparameter.The addition of the optional
isButtonparameter is correctly implemented, enhancing the function's flexibility.
184-193: Conditional rendering for button tooltips implemented correctly.The logic for handling
ColumnTypes.BUTTONwith theuseToolTipfunction is clear and effective.
|
Deploy-Preview-URL: https://ce-36925.dp.appsmith.com |
….com/skjameela/appsmith into external-contri/fix/button-tooltip-in-table_widgetV2
…o external-contri/fix/button-tooltip-in-table_widgetV2
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11512821324. |
|
Deploy-Preview-URL: https://ce-36925.dp.appsmith.com |
rahulbarwal
added
ok-to-test
…o external-contri/fix/button-tooltip-in-table_widgetV2
….com/skjameela/appsmith into external-contri/fix/button-tooltip-in-table_widgetV2
…o external-contri/fix/button-tooltip-in-table_widgetV2
…o external-contri/fix/button-tooltip-in-table_widgetV2
…o external-contri/fix/button-tooltip-in-table_widgetV2
|
Closing as original PR is merged. |
2 participants
Description
Issue: #10278
Original PR: #36865
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/5373
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11588397189
Commit: 44de49c
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 30 Oct 2024 09:07:02 UTC
Summary by CodeRabbit
New Features
RenderOptionWrapper,MenuCategoryWrapper, andTooltipContentWrapper.Bug Fixes
Refactor